From: Ian Jackson Date: Thu, 9 Jul 2015 16:28:48 +0000 (+0100) Subject: libxl: poll: Avoid fd deregistration race POLLNVAL crash X-Git-Tag: archive/raspbian/4.8.0-1+rpi1~1^2~2787 X-Git-Url: https://dgit.raspbian.org/%22http://www.example.com/cgi/%22/%22http:/www.example.com/cgi/%22?a=commitdiff_plain;h=681ce1681622a46d111cfdc4fc07e4cb565ae131;p=xen.git libxl: poll: Avoid fd deregistration race POLLNVAL crash It can happen that an fd is deregistered, and closed, and then a new fd opened, and reregistered, all while another thread is in poll(). If this happens poll might report POLLNVAL, but the event loop would think that the fd was supposed to have been valid, and then fail an assertion: libxl_event.c:1183: afterpoll_check_fd: Assertion `poller->fds_changed || !(fds[slot].revents & 0x020)' failed. We can't simply ignore POLLNVAL because if we have bugs which cause messed-up fds, it is a serious problem which we really need to detect. Instead, add extra tracking to spot when this possibility arises, and abort on POLLNVAL if we are sure that it is unexpected. Reported-by: Jim Fehlig Signed-off-by: Ian Jackson CC: Jim Fehlig Acked-by: Wei Liu Tested-by: Jim Fehlig --- diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c index b6f58f131b..3fe1b99001 100644 --- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -62,6 +62,7 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version, ctx->poller_app = 0; LIBXL_LIST_INIT(&ctx->pollers_event); LIBXL_LIST_INIT(&ctx->pollers_idle); + LIBXL_LIST_INIT(&ctx->pollers_fds_changed); LIBXL_LIST_INIT(&ctx->efds); LIBXL_TAILQ_INIT(&ctx->etimes); @@ -190,6 +191,7 @@ int libxl_ctx_free(libxl_ctx *ctx) libxl__poller_put(ctx, ctx->poller_app); ctx->poller_app = NULL; assert(LIBXL_LIST_EMPTY(&ctx->pollers_event)); + assert(LIBXL_LIST_EMPTY(&ctx->pollers_fds_changed)); libxl__poller *poller, *poller_tmp; LIBXL_LIST_FOREACH_SAFE(poller, &ctx->pollers_idle, entry, poller_tmp) { libxl__poller_dispose(poller); diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c index 942e02c471..8acecfa544 100644 --- a/tools/libxl/libxl_event.c +++ b/tools/libxl/libxl_event.c @@ -225,6 +225,7 @@ int libxl__ev_fd_modify(libxl__gc *gc, libxl__ev_fd *ev, short events) void libxl__ev_fd_deregister(libxl__gc *gc, libxl__ev_fd *ev) { CTX_LOCK; + libxl__poller *poller; if (!libxl__ev_fd_isregistered(ev)) { DBG("ev_fd=%p deregister unregistered",ev); @@ -237,6 +238,9 @@ void libxl__ev_fd_deregister(libxl__gc *gc, libxl__ev_fd *ev) LIBXL_LIST_REMOVE(ev, entry); ev->fd = -1; + LIBXL_LIST_FOREACH(poller, &CTX->pollers_fds_changed, fds_changed_entry) + poller->fds_changed = 1; + out: CTX_UNLOCK; } @@ -1110,6 +1114,8 @@ static int beforepoll_internal(libxl__gc *gc, libxl__poller *poller, *nfds_io = used; + poller->fds_changed = 0; + libxl__ev_time *etime = LIBXL_TAILQ_FIRST(&CTX->etimes); if (etime) { int our_timeout; @@ -1174,7 +1180,7 @@ static int afterpoll_check_fd(libxl__poller *poller, /* again, stale slot entry */ continue; - assert(!(fds[slot].revents & POLLNVAL)); + assert(poller->fds_changed || !(fds[slot].revents & POLLNVAL)); /* we mask in case requested events have changed */ int slot_revents = fds[slot].revents & events; @@ -1601,6 +1607,7 @@ int libxl__poller_init(libxl__gc *gc, libxl__poller *p) int rc; p->fd_polls = 0; p->fd_rindices = 0; + p->fds_changed = 0; rc = libxl__pipe_nonblock(CTX, p->wakeup_pipe); if (rc) goto out; @@ -1637,12 +1644,15 @@ libxl__poller *libxl__poller_get(libxl__gc *gc) } } + LIBXL_LIST_INSERT_HEAD(&CTX->pollers_fds_changed, p, + fds_changed_entry); return p; } void libxl__poller_put(libxl_ctx *ctx, libxl__poller *p) { if (!p) return; + LIBXL_LIST_REMOVE(p, fds_changed_entry); LIBXL_LIST_INSERT_HEAD(&ctx->pollers_idle, p, entry); } diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h index 97de141dac..2b6b2a0164 100644 --- a/tools/libxl/libxl_internal.h +++ b/tools/libxl/libxl_internal.h @@ -368,6 +368,18 @@ struct libxl__poller { int (*fd_rindices)[3]; /* see libxl_event.c:beforepoll_internal */ int wakeup_pipe[2]; /* 0 means no fd allocated */ + + /* + * We also use the poller to record whether any fds have been + * deregistered since we entered poll. Each poller which is not + * idle is on the list pollers_fds_changed. fds_changed is + * cleared by beforepoll, and tested by afterpoll. Whenever an fd + * event is deregistered, we set the fds_changed of all non-idle + * pollers. So afterpoll can tell whether any POLLNVAL is + * plausibly due to an fd being closed and reopened. + */ + LIBXL_LIST_ENTRY(libxl__poller) fds_changed_entry; + bool fds_changed; }; struct libxl__gc { @@ -409,6 +421,7 @@ struct libxl__ctx { libxl__poller *poller_app; /* libxl_osevent_beforepoll and _afterpoll */ LIBXL_LIST_HEAD(, libxl__poller) pollers_event, pollers_idle; + LIBXL_LIST_HEAD(, libxl__poller) pollers_fds_changed; LIBXL_SLIST_HEAD(libxl__osevent_hook_nexi, libxl__osevent_hook_nexus) hook_fd_nexi_idle, hook_timeout_nexi_idle;